RE: [Patch (resend) 5/5] cifs: Call MID callback before destroying transport

From: Long Li <longli@microsoft.com>
To: Pavel Shilovsky <piastryyy@gmail.com>
Cc: Steve French <sfrench@samba.org>,
	linux-cifs <linux-cifs@vger.kernel.org>,
	samba-technical <samba-technical@lists.samba.org>,
	Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: RE: [Patch (resend) 5/5] cifs: Call MID callback before destroying transport
Date: Tue, 14 May 2019 01:32:03 +0000	[thread overview]
Message-ID: <DM5PR21MB0635F7EBA2DEF3437F57A653CE080@DM5PR21MB0635.namprd21.prod.outlook.com> (raw)
In-Reply-To: <CAKywueTBsHuBOchj7ysL8S+pU=nL6dfF65YT9YZrVk74HUoRVQ@mail.gmail.com>

>>>-----Original Message-----
>>>From: Pavel Shilovsky <piastryyy@gmail.com>
>>>Sent: Thursday, May 9, 2019 11:01 AM
>>>To: Long Li <longli@microsoft.com>
>>>Cc: Steve French <sfrench@samba.org>; linux-cifs <linux-
>>>cifs@vger.kernel.org>; samba-technical <samba-technical@lists.samba.org>;
>>>Kernel Mailing List <linux-kernel@vger.kernel.org>
>>>Subject: Re: [Patch (resend) 5/5] cifs: Call MID callback before destroying
>>>transport
>>>
>>>пт, 5 апр. 2019 г. в 14:39, Long Li <longli@linuxonhyperv.com>:
>>>>
>>>> From: Long Li <longli@microsoft.com>
>>>>
>>>> When transport is being destroyed, it's possible that some processes
>>>> may hold memory registrations that need to be deregistred.
>>>>
>>>> Call them first so nobody is using transport resources, and it can be
>>>> destroyed.
>>>>
>>>> Signed-off-by: Long Li <longli@microsoft.com>
>>>> ---
>>>>  fs/cifs/connect.c | 36 +++++++++++++++++++-----------------
>>>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index
>>>> 33e4d98..084756cf 100644
>>>> --- a/fs/cifs/connect.c
>>>> +++ b/fs/cifs/connect.c
>>>> @@ -528,22 +528,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>>>         /* do not want to be sending data on a socket we are freeing */
>>>>         cifs_dbg(FYI, "%s: tearing down socket\n", __func__);
>>>>         mutex_lock(&server->srv_mutex);
>>>> -       if (server->ssocket) {
>>>> -               cifs_dbg(FYI, "State: 0x%x Flags: 0x%lx\n",
>>>> -                        server->ssocket->state, server->ssocket->flags);
>>>> -               kernel_sock_shutdown(server->ssocket, SHUT_WR);
>>>> -               cifs_dbg(FYI, "Post shutdown state: 0x%x Flags: 0x%lx\n",
>>>> -                        server->ssocket->state, server->ssocket->flags);
>>>> -               sock_release(server->ssocket);
>>>> -               server->ssocket = NULL;
>>>> -       } else if (cifs_rdma_enabled(server))
>>>> -               smbd_destroy(server);
>>>> -       server->sequence_number = 0;
>>>> -       server->session_estab = false;
>>>> -       kfree(server->session_key.response);
>>>> -       server->session_key.response = NULL;
>>>> -       server->session_key.len = 0;
>>>> -       server->lstrp = jiffies;
>>>>
>>>>         /* mark submitted MIDs for retry and issue callback */
>>>>         INIT_LIST_HEAD(&retry_list);
>>>> @@ -556,7 +540,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>>>                 list_move(&mid_entry->qhead, &retry_list);
>>>>         }
>>>>         spin_unlock(&GlobalMid_Lock);
>>>> -       mutex_unlock(&server->srv_mutex);
>>>>
>>>>         cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
>>>>         list_for_each_safe(tmp, tmp2, &retry_list) { @@ -565,6 +548,25
>>>> @@ cifs_reconnect(struct TCP_Server_Info *server)
>>>>                 mid_entry->callback(mid_entry);
>>>>         }
>>>
>>>The original call was issuing callbacks without holding srv_mutex - callbacks
>>>may take this mutex for its internal needs. With the proposed patch the
>>>code will deadlock.
>>>
>>>Also the idea of destroying the socket first is to allow possible retries (from
>>>callbacks) to return a proper error instead of trying to send anything through
>>>the reconnecting socket.

I will send a patch to revert this and follow your suggestion on putting smbd_destroy() to after all MIDs have been called. Your suggestion tested well.

Thanks

Long

>>>
>>>>
>>>> +       if (server->ssocket) {
>>>> +               cifs_dbg(FYI, "State: 0x%x Flags: 0x%lx\n",
>>>> +                        server->ssocket->state, server->ssocket->flags);
>>>> +               kernel_sock_shutdown(server->ssocket, SHUT_WR);
>>>> +               cifs_dbg(FYI, "Post shutdown state: 0x%x Flags: 0x%lx\n",
>>>> +                        server->ssocket->state, server->ssocket->flags);
>>>> +               sock_release(server->ssocket);
>>>> +               server->ssocket = NULL;
>>>> +       } else if (cifs_rdma_enabled(server))
>>>> +               smbd_destroy(server);
>>>
>>>If we need to call smbd_destroy() *after* callbacks, let's just move it alone
>>>without the rest of the code.
>>>
>>>
>>>> +       server->sequence_number = 0;
>>>> +       server->session_estab = false;
>>>> +       kfree(server->session_key.response);
>>>> +       server->session_key.response = NULL;
>>>> +       server->session_key.len = 0;
>>>> +       server->lstrp = jiffies;
>>>> +
>>>> +       mutex_unlock(&server->srv_mutex);
>>>> +
>>>>         do {
>>>>                 try_to_freeze();
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>>
>>>
>>>--
>>>Best regards,
>>>Pavel Shilovsky