Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
I've observed a very nasty NULL pointer dereference with NVME drives on hot-unplug [1]. While I haven't gone in the details of this change, it does fix the NULL dereference I've been seeing. Alex [1] http://lists.infradead.org/pipermail/linux-nvme/2018-April/016623.html
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
Hello, Bart.
On Mon, Apr 09, 2018 at 09:30:27PM +, Bart Van Assche wrote:
> On Mon, 2018-04-09 at 11:56 -0700, t...@kernel.org wrote:
> > On Mon, Apr 09, 2018 at 05:03:05PM +, Bart Van Assche wrote:
> > > exist today in the blk-mq timeout handling code cannot be fixed completely
> > > using RCU only.
> >
> > I really don't think that is that complicated. Let's first confirm
> > the race fix and get to narrowing / closing that window.
>
> Two months ago it was reported for the first time that commit 1d9bd5161ba3
> ("blk-mq: replace timeout synchronization with a RCU and generation based
> scheme") introduces a regression. Since that report nobody has posted a
> patch that fixes all races related to blk-mq timeout handling and that only
The two patches using RCU were posted a long time ago. It was just
that the repro that only you had at the time didn't work anymore so we
couldn't confirm the fix. If we now have a different repro, awesome.
Let's see whether the fix works.
> uses RCU. If you want to continue working on this that's fine with me. But
> since my opinion is that it is impossible to fix these races using RCU only
> I will continue working on an alternative approach. See also "[PATCH]
> blk-mq: Fix a race between resetting the timer and completion handling"
> (https://www.mail-archive.com/linux-block@vger.kernel.org/msg18089.html).
ISTR discussing that patch earlier. Didn't the RCU based fix get
posted after that discussion?
Thanks.
--
tejun
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On Mon, 2018-04-09 at 11:56 -0700, t...@kernel.org wrote:
> On Mon, Apr 09, 2018 at 05:03:05PM +, Bart Van Assche wrote:
> > exist today in the blk-mq timeout handling code cannot be fixed completely
> > using RCU only.
>
> I really don't think that is that complicated. Let's first confirm
> the race fix and get to narrowing / closing that window.
Hello Tejun,
Two months ago it was reported for the first time that commit 1d9bd5161ba3
("blk-mq: replace timeout synchronization with a RCU and generation based
scheme") introduces a regression. Since that report nobody has posted a
patch that fixes all races related to blk-mq timeout handling and that only
uses RCU. If you want to continue working on this that's fine with me. But
since my opinion is that it is impossible to fix these races using RCU only
I will continue working on an alternative approach. See also "[PATCH]
blk-mq: Fix a race between resetting the timer and completion handling"
(https://www.mail-archive.com/linux-block@vger.kernel.org/msg18089.html).
Thanks,
Bart.
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On Mon, 2018-04-09 at 09:47 -0700, Tejun Heo wrote: > On Sun, Apr 08, 2018 at 10:20:38PM -0700, Bart Van Assche wrote: > > If a completion occurs after blk_mq_rq_timed_out() has reset > > rq->aborted_gstate and the request is again in flight when the timeout > > expires then a request will be completed twice: a first time by the > > timeout handler and a second time when the regular completion occurs. > > Are we still talking about the same BLK_EH_RESET_TIMER case? This can > be solved by the two patches which rcu-synchronizes the hand-over to > normal completion path, right? Hello Tejun, My opinion is not only that the two patches that you posted recently do not fix all the races that are fixed by this patch but also that the races that exist today in the blk-mq timeout handling code cannot be fixed completely using RCU only. > > Additionally, the blk-mq timeout handling code ignores completions that > > occur after blk_mq_check_expired() has been called and before > > blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver > > timeout handler always returns BLK_EH_RESET_TIMER then the result will > > be that the request never terminates. > > And this is the same race window which was always there, right? I > really don't think reducing or closing this window requires full > synchronization. That race window did not exist in the legacy block layer. I don't think we should tolerate such a race window in the blk-mq code either. If a request does not get completed that leads to unkillable processes or hanging kernel code. Such issues are hard to identify when reported by users. I think we should fix these races before these cause more trouble to Linux users. Thanks, Bart.
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
Hello, Sagi. On Mon, Apr 09, 2018 at 11:37:15AM +0300, Sagi Grimberg wrote: > > >If a completion occurs after blk_mq_rq_timed_out() has reset > >rq->aborted_gstate and the request is again in flight when the timeout > >expires then a request will be completed twice: a first time by the > >timeout handler and a second time when the regular completion occurs. > > > >Additionally, the blk-mq timeout handling code ignores completions that > >occur after blk_mq_check_expired() has been called and before > >blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver > >timeout handler always returns BLK_EH_RESET_TIMER then the result will > >be that the request never terminates. > > OK, now I understand how we can complete twice. Israel, can you verify > this patch solves your double completion problem? > > Given that it is, the change log of your patches should be modified to > the original bug report it solves. > > Thread starts here: > http://lists.infradead.org/pipermail/linux-nvme/2018-February/015848.html Can you please see whether the following two patches fix the problem you've been seeing? http://lkml.kernel.org/r/20180402190053.gc388...@devbig577.frc2.facebook.com http://lkml.kernel.org/r/20180402190120.gd388...@devbig577.frc2.facebook.com Thanks. -- tejun
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
Hey, Bart. On Sun, Apr 08, 2018 at 10:20:38PM -0700, Bart Van Assche wrote: > If a completion occurs after blk_mq_rq_timed_out() has reset > rq->aborted_gstate and the request is again in flight when the timeout > expires then a request will be completed twice: a first time by the > timeout handler and a second time when the regular completion occurs. Are we still talking about the same BLK_EH_RESET_TIMER case? This can be solved by the two patches which rcu-synchronizes the hand-over to normal completion path, right? > Additionally, the blk-mq timeout handling code ignores completions that > occur after blk_mq_check_expired() has been called and before > blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver > timeout handler always returns BLK_EH_RESET_TIMER then the result will > be that the request never terminates. And this is the same race window which was always there, right? I really don't think reducing or closing this window requires full synchronization. Thanks. -- tejun
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On 4/9/2018 11:37 AM, Sagi Grimberg wrote: If a completion occurs after blk_mq_rq_timed_out() has reset rq->aborted_gstate and the request is again in flight when the timeout expires then a request will be completed twice: a first time by the timeout handler and a second time when the regular completion occurs. Additionally, the blk-mq timeout handling code ignores completions that occur after blk_mq_check_expired() has been called and before blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver timeout handler always returns BLK_EH_RESET_TIMER then the result will be that the request never terminates. OK, now I understand how we can complete twice. Israel, can you verify this patch solves your double completion problem? I just verified that this commit fix the double completion problem. We still need my patches to fix the original bug report. Given that it is, the change log of your patches should be modified to the original bug report it solves. Thread starts here: http://lists.infradead.org/pipermail/linux-nvme/2018-February/015848.html
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On 4/9/18 8:58 AM, Bart Van Assche wrote: > On Mon, 2018-04-09 at 11:37 +0200, Christoph Hellwig wrote: >> This looks sensible, but I'm worried about taking a whole spinlock >> for every request completion, including irq disabling. However it seems >> like your new updated pattern would fit use of cmpxchg() very nicely. > > Hello Christoph, > > Thanks for the review. I had a look at the spin lock implementation on > x86 and apparently on x86 spin locks are implemented as qspinlocks > (include/asm-generic/qspinlock.h). queued_spin_lock() already uses > atomic_cmpxchg_acquire(). Are you sure that replacing the spin lock > by cmpxchg() will yield a performance improvement? It's definitely worth a shot, especially since this looks like a clear cut case for cmpxchg(). Additionally, it also kills the local irq disable. Conversion should be trivial and we can do some perf testing around that. Neither solution really makes me happy though, the prospect of either kind of synchronization cost isn't appealing at all. I'll have to ponder this a lot more, but it would be ideal if we could shift that cost to the extremely unlikely case of a timeout triggering rather than add the cost upfront to EVERY request. -- Jens Axboe
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On Mon, 2018-04-09 at 11:37 +0200, Christoph Hellwig wrote: > This looks sensible, but I'm worried about taking a whole spinlock > for every request completion, including irq disabling. However it seems > like your new updated pattern would fit use of cmpxchg() very nicely. Hello Christoph, Thanks for the review. I had a look at the spin lock implementation on x86 and apparently on x86 spin locks are implemented as qspinlocks (include/asm-generic/qspinlock.h). queued_spin_lock() already uses atomic_cmpxchg_acquire(). Are you sure that replacing the spin lock by cmpxchg() will yield a performance improvement? Thanks, Bart.
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
On Mon, 2018-04-09 at 11:37 +0300, Sagi Grimberg wrote: > > If a completion occurs after blk_mq_rq_timed_out() has reset > > rq->aborted_gstate and the request is again in flight when the timeout > > expires then a request will be completed twice: a first time by the > > timeout handler and a second time when the regular completion occurs. > > > > Additionally, the blk-mq timeout handling code ignores completions that > > occur after blk_mq_check_expired() has been called and before > > blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver > > timeout handler always returns BLK_EH_RESET_TIMER then the result will > > be that the request never terminates. > > OK, now I understand how we can complete twice. Israel, can you verify > this patch solves your double completion problem? > > Given that it is, the change log of your patches should be modified to > the original bug report it solves. > > Thread starts here: > http://lists.infradead.org/pipermail/linux-nvme/2018-February/015848.html Hello Sagi, I will wait until it has been verified whether or not this patch fixes the "nvme-rdma corrupts memory upon timeout" issue before adding a reference to that issue in the description of this patch. Thanks, Bart.
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
This looks sensible, but I'm worried about taking a whole spinlock for every request completion, including irq disabling. However it seems like your new updated pattern would fit use of cmpxchg() very nicely.
Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code
If a completion occurs after blk_mq_rq_timed_out() has reset rq->aborted_gstate and the request is again in flight when the timeout expires then a request will be completed twice: a first time by the timeout handler and a second time when the regular completion occurs. Additionally, the blk-mq timeout handling code ignores completions that occur after blk_mq_check_expired() has been called and before blk_mq_rq_timed_out() has reset rq->aborted_gstate. If a block driver timeout handler always returns BLK_EH_RESET_TIMER then the result will be that the request never terminates. OK, now I understand how we can complete twice. Israel, can you verify this patch solves your double completion problem? Given that it is, the change log of your patches should be modified to the original bug report it solves. Thread starts here: http://lists.infradead.org/pipermail/linux-nvme/2018-February/015848.html
