From: Chuck Lever <chuck.lever@oracle.com>
To: Dan Aloni <dan@kernelim.com>
Cc: linux-rdma@vger.kernel.org,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] xprtrdma: fix EP destruction logic
Date: Fri, 26 Jun 2020 11:22:37 -0400 [thread overview]
Message-ID: <9A5CDAC4-69AF-45F2-8574-F749499CA500@oracle.com> (raw)
In-Reply-To: <20200626151052.6cckaquyu7k3nd6b@gmail.com>
> On Jun 26, 2020, at 11:10 AM, Dan Aloni <dan@kernelim.com> wrote:
>
> On Fri, Jun 26, 2020 at 08:56:41AM -0400, Chuck Lever wrote:
>>> On Jun 26, 2020, at 3:10 AM, Dan Aloni <dan@kernelim.com> wrote:
> [..]
>>> - Add a mutex in `rpcrdma_ep_destroy` to guard against concurrent calls
>>> to `rpcrdma_xprt_disconnect` coming from either `rpcrdma_xprt_connect`
>>> or `xprt_rdma_close`.
>>
>> NAK. The RPC client provides appropriate exclusion, please let's not
>> add more serialization that can introduce further deadlocks.
>
> It appeared to me that this exclusion does not works well. As for my
> considerations, if I am not mistaken from analyzing crashes I've
> seen:
>
> -> xprt_autoclose (running on xprtiod)
> -> xprt->ops->close
> -> xprt_rdma_close
> -> rpcrdma_xprt_disconnect
>
> and:
>
> -> xprt_rdma_connect_worker (running on xprtiod)
> -> rpcrdma_xprt_connect
> -> rpcrdma_xprt_disconnect
>
> I understand the rationale or at least the aim that `close` and
> `connect` ops should not be concurrent on the same `xprt`, however:
>
> * `xprt_force_disconnect`, is called from various places, queues
> a call to `xprt_autoclose` to the background on `xprtiod` workqueue item,
> conditioned that `!XPRT_LOCKED` which is the case for connect that went
> to the background.
> * `xprt_rdma_connect` also sends `xprt_rdma_connect_worker` as an `xprtiod`
> workqueue item, unconditionally.
>
> So we have two work items that can run in parallel, and I don't see
> clear gating on this from the code.
If close and connect are being called concurrently on the same xprt,
then there is a bug in the generic RPC xprt code. I don't believe
that to be the case here.
If xprtrdma invokes force_disconnect during connect processing,
XPRT_LOCKED should be held and the close should be delayed.
> Maybe there's a simpler fix for this. Perhaps a
> `cancel_delayed_work_sync(&r_xprt->rx_connect_worker);` is appropriate
> in `xprt_rdma_close`?
There are simpler, less deadlock-prone, and more justifiable fixes.
Please stand by, I will take care of this today.
--
Chuck Lever
next prev parent reply other threads:[~2020-06-26 15:22 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-21 14:59 [PATCH] xprtrdma: Ensure connect worker is awoken after connect error Chuck Lever
2020-06-25 19:19 ` Chuck Lever
2020-06-26 7:10 ` [PATCH] xprtrdma: fix EP destruction logic Dan Aloni
2020-06-26 12:56 ` Chuck Lever
2020-06-26 15:10 ` Dan Aloni
2020-06-26 15:22 ` Chuck Lever [this message]
2020-06-26 7:17 ` [PATCH] xprtrdma: Ensure connect worker is awoken after connect error Dan Aloni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9A5CDAC4-69AF-45F2-8574-F749499CA500@oracle.com \
--to=chuck.lever@oracle.com \
--cc=dan@kernelim.com \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).