Lee Duncan <lduncan@xxxxxxxx> writes:
> On 5/7/20 10:59 PM, Gabriel Krisman Bertazi wrote:
>> iscsi suffers from a deadlock in case a management command submitted via
>> the netlink socket sleeps on an allocation while holding the
>> rx_queue_mutex, if that allocation causes a memory reclaim that
>> writebacks to a failed iscsi device. Then, the recovery procedure can
>> never make progress to recover the failed disk or abort outstanding IO
>> operations to complete the reclaim (since rx_queue_mutex is locked),
>> thus locking the system.
>>
>> Nevertheless, just marking all allocations under rx_queue_mutex as
>> GFP_NOIO (or locking the userspace process with something like
>> PF_MEMALLOC_NOIO) is not enough, since the iscsi command code relies on
>> other subsystems that try to grab locked mutexes, whose threads are
>> GFP_IO, leading to the same deadlock. One instance where this situation
>> can be observed is in the backtraces below, stitched from multiple bugs
>> reports, involving the kobj uevent sent when a session is created.
>>
>> The root of the problem is not the fact that iscsi does GFP_IO
>> allocations, that is acceptable. The actual problem is that
>> rx_queue_mutex has a very large granularity, covering every unrelated
>> netlink command execution at the same time as the error recovery path.
>>
>> The proposed fix leverages the recently added mechanism to stop failed
>> connections from the kernel, by enabling it to execute even though a
>> management command from the netlink socket is being run (rx_queue_mutex
>> is held), provided that the command is known to be safe. It splits the
>> rx_queue_mutex in two mutexes, one protecting from concurrent command
>> execution from the netlink socket, and one protecting stop_conn from
>> racing with other connection management operations that might conflict
>> with it.
>>
>> It is not very pretty, but it is the simplest way to resolve the
>> deadlock. I considered making it a lock per connection, but some
>> external mutex would still be needed to deal with iscsi_if_destroy_conn.
>>
>> The patch was tested by forcing a memory shrinker (unrelated, but used
>> bufio/dm-verity) to reclaim ISCSI pages every time
>> ISCSI_UEVENT_CREATE_SESSION happens, which is reasonable to simulate
>> reclaims that might happen with GFP_KERNEL on that path. Then, a faulty
>> hung target causes a connection to fail during intensive IO, at the same
>> time a new session is added by iscsid.
>>
>> The following stacktraces are stiches from several bug reports, showing
>> a case where the deadlock can happen.
>>
>> iSCSI-write
>> holding: rx_queue_mutex
>> waiting: uevent_sock_mutex
>>
>> kobject_uevent_env+0x1bd/0x419
>> kobject_uevent+0xb/0xd
>> device_add+0x48a/0x678
>> scsi_add_host_with_dma+0xc5/0x22d
>> iscsi_host_add+0x53/0x55
>> iscsi_sw_tcp_session_create+0xa6/0x129
>> iscsi_if_rx+0x100/0x1247
>> netlink_unicast+0x213/0x4f0
>> netlink_sendmsg+0x230/0x3c0
>>
>> iscsi_fail iscsi_conn_failure
>> waiting: rx_queue_mutex
>>
>> schedule_preempt_disabled+0x325/0x734
>> __mutex_lock_slowpath+0x18b/0x230
>> mutex_lock+0x22/0x40
>> iscsi_conn_failure+0x42/0x149
>> worker_thread+0x24a/0xbc0
>>
>> EventManager_
>> holding: uevent_sock_mutex
>> waiting: dm_bufio_client->lock
>>
>> dm_bufio_lock+0xe/0x10
>> shrink+0x34/0xf7
>> shrink_slab+0x177/0x5d0
>> do_try_to_free_pages+0x129/0x470
>> try_to_free_mem_cgroup_pages+0x14f/0x210
>> memcg_kmem_newpage_charge+0xa6d/0x13b0
>> __alloc_pages_nodemask+0x4a3/0x1a70
>> fallback_alloc+0x1b2/0x36c
>> __kmalloc_node_track_caller+0xb9/0x10d0
>> __alloc_skb+0x83/0x2f0
>> kobject_uevent_env+0x26b/0x419
>> dm_kobject_uevent+0x70/0x79
>> dev_suspend+0x1a9/0x1e7
>> ctl_ioctl+0x3e9/0x411
>> dm_ctl_ioctl+0x13/0x17
>> do_vfs_ioctl+0xb3/0x460
>> SyS_ioctl+0x5e/0x90
>>
>> MemcgReclaimerD"
>> holding: dm_bufio_client->lock
>> waiting: stuck io to finish (needs iscsi_fail thread to progress)
>>
>> schedule at ffffffffbd603618
>> io_schedule at ffffffffbd603ba4
>> do_io_schedule at ffffffffbdaf0d94
>> __wait_on_bit at ffffffffbd6008a6
>> out_of_line_wait_on_bit at ffffffffbd600960
>> wait_on_bit.constprop.10 at ffffffffbdaf0f17
>> __make_buffer_clean at ffffffffbdaf18ba
>> __cleanup_old_buffer at ffffffffbdaf192f
>> shrink at ffffffffbdaf19fd
>> do_shrink_slab at ffffffffbd6ec000
>> shrink_slab at ffffffffbd6ec24a
>> do_try_to_free_pages at ffffffffbd6eda09
>> try_to_free_mem_cgroup_pages at ffffffffbd6ede7e
>> mem_cgroup_resize_limit at ffffffffbd7024c0
>> mem_cgroup_write at ffffffffbd703149
>> cgroup_file_write at ffffffffbd6d9c6e
>> sys_write at ffffffffbd6662ea
>> system_call_fastpath at ffffffffbdbc34a2
>>
>> Reported-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
>> ---
>> drivers/scsi/scsi_transport_iscsi.c | 67 +++++++++++++++++++++--------
>> 1 file changed, 49 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>> index 17a45716a0fe..d99c17306dff 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -1616,6 +1616,12 @@ static DECLARE_TRANSPORT_CLASS(iscsi_connection_class,
>> static struct sock *nls;
>> static DEFINE_MUTEX(rx_queue_mutex);
>>
>> +/*
>> + * conn_mutex protects the {start,bind,stop,destroy}_conn from racing
>> + * against the kernel stop_connection recovery mechanism
>> + */
>> +static DEFINE_MUTEX(conn_mutex);
>> +
>> static LIST_HEAD(sesslist);
>> static LIST_HEAD(sessdestroylist);
>> static DEFINE_SPINLOCK(sesslock);
>> @@ -2442,6 +2448,32 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
>> }
>> EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
>>
>> +/*
>> + * This can be called without the rx_queue_mutex, if invoked by the kernel
>> + * stop work. But, in that case, it is guaranteed not to race with
>> + * iscsi_destroy by conn_mutex.
>> + */
>> +static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
>> +{
>> + /*
>> + * It is important that this path doesn't rely on
>> + * rx_queue_mutex, otherwise, a thread doing allocation on a
>> + * start_session/start_connection could sleep waiting on a
>> + * writeback to a failed iscsi device, that cannot be recovered
>> + * because the lock is held. If we don't hold it here, the
>> + * kernel stop_conn_work_fn has a chance to stop the broken
>> + * session and resolve the allocation.
>> + *
>> + * Still, the user invoked .stop_conn() needs to be serialized
>> + * with stop_conn_work_fn by a private mutex. Not pretty, but
>> + * it works.
>> + */
>> + mutex_lock(&conn_mutex);
>> + conn->transport->stop_conn(conn, flag);
>> + mutex_unlock(&conn_mutex);
>> +
>> +}
>> +
>> static void stop_conn_work_fn(struct work_struct *work)
>> {
>> struct iscsi_cls_conn *conn, *tmp;
>> @@ -2460,30 +2492,17 @@ static void stop_conn_work_fn(struct work_struct *work)
>> uint32_t sid = iscsi_conn_get_sid(conn);
>> struct iscsi_cls_session *session;
>>
>> - mutex_lock(&rx_queue_mutex);
>> -
>> session = iscsi_session_lookup(sid);
>> if (session) {
>> if (system_state != SYSTEM_RUNNING) {
>> session->recovery_tmo = 0;
>> - conn->transport->stop_conn(conn,
>> - STOP_CONN_TERM);
>> + iscsi_if_stop_conn(conn, STOP_CONN_TERM);
>> } else {
>> - conn->transport->stop_conn(conn,
>> - STOP_CONN_RECOVER);
>> + iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
>> }
>> }
>>
>> list_del_init(&conn->conn_list_err);
>> -
>> - mutex_unlock(&rx_queue_mutex);
>> -
>> - /* we don't want to hold rx_queue_mutex for too long,
>> - * for instance if many conns failed at the same time,
>> - * since this stall other iscsi maintenance operations.
>> - * Give other users a chance to proceed.
>> - */
>> - cond_resched();
>> }
>> }
>
> I'm curious about why you removed the cond_resched() here. Is it because
> it is no longer needed, with shorter (mutex) waiting time?
>
Hi Lee,
My thought was that the main reason for cond_resched here was to give a
chance for other iscsi maintenance operations to run. But, since
conn_mutex, differently from rx_queue_mutex, won't stop most of the
operations, it felt unnecessary. If you disagree, I can submit a v2
that doesn't change it.
>>
>> @@ -2843,8 +2862,11 @@ iscsi_if_destroy_conn(struct iscsi_transport *transport, struct iscsi_uevent *ev
>> spin_unlock_irqrestore(&connlock, flags);
>>
>> ISCSI_DBG_TRANS_CONN(conn, "Destroying transport conn\n");
>> +
>> + mutex_lock(&conn_mutex);
>> if (transport->destroy_conn)
>> transport->destroy_conn(conn);
>> + mutex_unlock(&conn_mutex);
>>
>> return 0;
>> }
>> @@ -3686,9 +3708,12 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
>> break;
>> }
>>
>> + mutex_lock(&conn_mutex);
>> ev->r.retcode = transport->bind_conn(session, conn,
>> ev->u.b_conn.transport_eph,
>> ev->u.b_conn.is_leading);
>> + mutex_unlock(&conn_mutex);
>> +
>> if (ev->r.retcode || !transport->ep_connect)
>> break;
>>
>> @@ -3709,25 +3734,31 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
>> break;
>> case ISCSI_UEVENT_START_CONN:
>> conn = iscsi_conn_lookup(ev->u.start_conn.sid, ev->u.start_conn.cid);
>> - if (conn)
>> + if (conn) {
>> + mutex_lock(&conn_mutex);
>> ev->r.retcode = transport->start_conn(conn);
>> + mutex_unlock(&conn_mutex);
>> + }
>> else
>> err = -EINVAL;
>> break;
>> case ISCSI_UEVENT_STOP_CONN:
>> conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
>> if (conn)
>> - transport->stop_conn(conn, ev->u.stop_conn.flag);
>> + iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
>> else
>> err = -EINVAL;
>> break;
>> case ISCSI_UEVENT_SEND_PDU:
>> conn = iscsi_conn_lookup(ev->u.send_pdu.sid, ev->u.send_pdu.cid);
>> - if (conn)
>> + if (conn) {
>> + mutex_lock(&conn_mutex);
>> ev->r.retcode = transport->send_pdu(conn,
>> (struct iscsi_hdr*)((char*)ev + sizeof(*ev)),
>> (char*)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
>> ev->u.send_pdu.data_size);
>> + mutex_unlock(&conn_mutex);
>> + }
>> else
>> err = -EINVAL;
>> break;
>>
>
> My question above is for my own information, so I'll still say:
>
> Reviewed-by: Lee Duncan <lduncan@xxxxxxxx>
--
Gabriel Krisman Bertazi
