[PATCH RT] rtmutex: Flush block plug on __down_read()

[PATCH RT] rtmutex: Flush block plug on __down_read()

[Scott Wood]

__down_read() bypasses the rtmutex frontend to call
rt_mutex_slowlock_locked() directly, and thus it needs to call
blk_schedule_flush_flug() itself.

Signed-off-by: Scott Wood <swood@redhat.com>
---
 kernel/locking/rwsem-rt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/locking/rwsem-rt.c b/kernel/locking/rwsem-rt.c
index 660e22caf709..f73cd4eda5cd 100644
--- a/kernel/locking/rwsem-rt.c
+++ b/kernel/locking/rwsem-rt.c
@@ -1,5 +1,6 @@
 /*
  */
+#include <linux/blkdev.h>
 #include <linux/rwsem.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/signal.h>
@@ -88,6 +89,15 @@ static int __sched __down_read_common(struct rw_semaphore *sem, int state)
 	if (__down_read_trylock(sem))
 		return 0;
 
+	/*
+	 * If sem->rtmutex blocks, the function sched_submit_work will not
+	 * call blk_schedule_flush_plug (because tsk_is_pi_blocked would be
+	 * true).  We must call blk_schedule_flush_plug here; if we don't
+	 * call it, an I/O deadlock may occur.
+	 */
+	if (unlikely(blk_needs_flush_plug(current)))
+		blk_schedule_flush_plug(current);
+
 	might_sleep();
 	raw_spin_lock_irq(&m->wait_lock);
 	/*
-- 
1.8.3.1

Re: [PATCH RT] rtmutex: Flush block plug on __down_read()

[Sebastian Andrzej Siewior]

On 2019-01-04 15:33:21 [-0500], Scott Wood wrote:
> __down_read() bypasses the rtmutex frontend to call
> rt_mutex_slowlock_locked() directly, and thus it needs to call
> blk_schedule_flush_flug() itself.

we don't do this in the spin_lock() case because !RT doesn't do it. We
do it for rtmutex because !RT does it for mutex.
Now I can't remember why this was skipped for a rw_sem since it is
performed for !RT as part of the schedule() invocation.

If I don't come up with a plausible explanation then I will apply this
plus a hunk for the __down_write_common() case which should also be
required (right?).

> Signed-off-by: Scott Wood <swood@redhat.com>
> ---
>  kernel/locking/rwsem-rt.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/locking/rwsem-rt.c b/kernel/locking/rwsem-rt.c
> index 660e22caf709..f73cd4eda5cd 100644
> --- a/kernel/locking/rwsem-rt.c
> +++ b/kernel/locking/rwsem-rt.c
> @@ -1,5 +1,6 @@
>  /*
>   */
> +#include <linux/blkdev.h>
>  #include <linux/rwsem.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched/signal.h>
> @@ -88,6 +89,15 @@ static int __sched __down_read_common(struct rw_semaphore *sem, int state)
>  	if (__down_read_trylock(sem))
>  		return 0;
>  
> +	/*
> +	 * If sem->rtmutex blocks, the function sched_submit_work will not
> +	 * call blk_schedule_flush_plug (because tsk_is_pi_blocked would be
> +	 * true).  We must call blk_schedule_flush_plug here; if we don't
> +	 * call it, an I/O deadlock may occur.
> +	 */
> +	if (unlikely(blk_needs_flush_plug(current)))
> +		blk_schedule_flush_plug(current);
> +
>  	might_sleep();
>  	raw_spin_lock_irq(&m->wait_lock);
>  	/*

Sebastian

Re: [PATCH RT] rtmutex: Flush block plug on __down_read()

[Scott Wood]

On Mon, 2019-01-07 at 17:50 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-01-04 15:33:21 [-0500], Scott Wood wrote:
> > __down_read() bypasses the rtmutex frontend to call
> > rt_mutex_slowlock_locked() directly, and thus it needs to call
> > blk_schedule_flush_flug() itself.
> 
> we don't do this in the spin_lock() case because !RT doesn't do it.

And because spin_lock() is called inside the flush path.

>  We
> do it for rtmutex because !RT does it for mutex.
> Now I can't remember why this was skipped for a rw_sem since it is
> performed for !RT as part of the schedule() invocation.

Without this we were seeing XFS hangs on our internal kernel.  I wasn't able
to reproduce it on a newer kernel, but it's very timing-dependant so I
wouldn't read too much into that.

> If I don't come up with a plausible explanation then I will apply this
> plus a hunk for the __down_write_common() case which should also be
> required (right?).

I don't think it's needed, as it doesn't call into the rtmutex code via a
backdoor.  When blocking on sem->rtmutex, rt_mutex_fastlock() will call the
flush.  When blocking with a direct call to schedule(), tsk_is_pi_blocked()
will not be true, and thus schedule() will do the flush via
sched_submit_work().

-Scott

Re: [PATCH RT] rtmutex: Flush block plug on __down_read()

[Sebastian Andrzej Siewior]

On 2019-01-08 13:19:47 [-0600], Scott Wood wrote:
> >  We
> > do it for rtmutex because !RT does it for mutex.
> > Now I can't remember why this was skipped for a rw_sem since it is
> > performed for !RT as part of the schedule() invocation.
> 
> Without this we were seeing XFS hangs on our internal kernel.  I wasn't able
> to reproduce it on a newer kernel, but it's very timing-dependant so I
> wouldn't read too much into that.

In this case I don't care much if you can reproduce this or not. The
point is that !RT does this so RT should do it, too. And back then (when
this piece was added) we wanted to copy the !RT behaviour and somehow
missed that piece.

> > If I don't come up with a plausible explanation then I will apply this
> > plus a hunk for the __down_write_common() case which should also be
> > required (right?).
> 
> I don't think it's needed, as it doesn't call into the rtmutex code via a
> backdoor.  When blocking on sem->rtmutex, rt_mutex_fastlock() will call the
> flush.  When blocking with a direct call to schedule(), tsk_is_pi_blocked()
> will not be true, and thus schedule() will do the flush via
> sched_submit_work().

a backdoor :) But yes, in each case we invoke schedule() so it is good.
Okay then. Let me apply it.
Thank you.

> -Scott

Sebastian

Re: [PATCH RT] rtmutex: Flush block plug on __down_read()

[Sebastian Andrzej Siewior]

On 2019-01-04 15:33:21 [-0500], Scott Wood wrote:
> +	/*
> +	 * If sem->rtmutex blocks, the function sched_submit_work will not
> +	 * call blk_schedule_flush_plug (because tsk_is_pi_blocked would be
> +	 * true).  We must call blk_schedule_flush_plug here; if we don't
> +	 * call it, an I/O deadlock may occur.
> +	 */

I kept the original comment plus I replaced device-mapper with I/O (the
patch said already that it is not device-mapper specific and may occur
with ext4 as well).

Sebastian