[PATCH 0/2] block: remove retry loop

[PATCH 0/2] block: remove retry loop

[John Ogness]

Hello,

This series removes a retry loop in the ioc reverse-order
double lock dance, replacing it with an implementation that
uses guaranteed forward progress.

While at it, it also removes the nested spinlock usage,
which no longer applies since CFQ is gone.

John Ogness (2):
  block: remove unnecessary ioc nested locking
  block: remove retry loop in ioc_release_fn()

 block/blk-ioc.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

-- 
2.20.1

[PATCH 1/2] block: remove unnecessary ioc nested locking

[John Ogness]

The legacy CFQ IO scheduler could call put_io_context() in its exit_icq()
elevator callback. This led to a lockdep warning, which was fixed in
commit d8c66c5d5924 ("block: fix lockdep warning on io_context release
put_io_context()") by using a nested subclass for the ioc spinlock.
However, with commit f382fb0bcef4 ("block: remove legacy IO schedulers")
the CFQ IO scheduler no longer exists.

The BFQ IO scheduler also implements the exit_icq() elevator callback but
does not call put_io_context().

The nested subclass for the ioc spinlock is no longer needed. Since it
existed as an exception and no longer applies, remove the nested subclass
usage.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 block/blk-ioc.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 9df50fb507ca..5dbcfa1b872e 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -96,15 +96,7 @@ static void ioc_release_fn(struct work_struct *work)
 {
 	struct io_context *ioc = container_of(work, struct io_context,
 					      release_work);
-	unsigned long flags;
-
-	/*
-	 * Exiting icq may call into put_io_context() through elevator
-	 * which will trigger lockdep warning.  The ioc's are guaranteed to
-	 * be different, use a different locking subclass here.  Use
-	 * irqsave variant as there's no spin_lock_irq_nested().
-	 */
-	spin_lock_irqsave_nested(&ioc->lock, flags, 1);
+	spin_lock_irq(&ioc->lock);
 
 	while (!hlist_empty(&ioc->icq_list)) {
 		struct io_cq *icq = hlist_entry(ioc->icq_list.first,
@@ -115,13 +107,13 @@ static void ioc_release_fn(struct work_struct *work)
 			ioc_destroy_icq(icq);
 			spin_unlock(&q->queue_lock);
 		} else {
-			spin_unlock_irqrestore(&ioc->lock, flags);
+			spin_unlock_irq(&ioc->lock);
 			cpu_relax();
-			spin_lock_irqsave_nested(&ioc->lock, flags, 1);
+			spin_lock_irq(&ioc->lock);
 		}
 	}
 
-	spin_unlock_irqrestore(&ioc->lock, flags);
+	spin_unlock_irq(&ioc->lock);
 
 	kmem_cache_free(iocontext_cachep, ioc);
 }
@@ -170,7 +162,6 @@ void put_io_context(struct io_context *ioc)
  */
 void put_io_context_active(struct io_context *ioc)
 {
-	unsigned long flags;
 	struct io_cq *icq;
 
 	if (!atomic_dec_and_test(&ioc->active_ref)) {
@@ -178,19 +169,14 @@ void put_io_context_active(struct io_context *ioc)
 		return;
 	}
 
-	/*
-	 * Need ioc lock to walk icq_list and q lock to exit icq.  Perform
-	 * reverse double locking.  Read comment in ioc_release_fn() for
-	 * explanation on the nested locking annotation.
-	 */
-	spin_lock_irqsave_nested(&ioc->lock, flags, 1);
+	spin_lock_irq(&ioc->lock);
 	hlist_for_each_entry(icq, &ioc->icq_list, ioc_node) {
 		if (icq->flags & ICQ_EXITED)
 			continue;
 
 		ioc_exit_icq(icq);
 	}
-	spin_unlock_irqrestore(&ioc->lock, flags);
+	spin_unlock_irq(&ioc->lock);
 
 	put_io_context(ioc);
 }
-- 
2.20.1

[PATCH 2/2] block: remove retry loop in ioc_release_fn()

[John Ogness]

The reverse-order double lock dance in ioc_release_fn() is using a
retry loop. This is a problem on PREEMPT_RT because it could preempt
the task that would release q->queue_lock and thus live lock in the
retry loop.

RCU is already managing the freeing of the request queue and icq. If
the trylock fails, use RCU to guarantee that the request queue and
icq are not freed and re-acquire the locks in the correct order,
allowing forward progress.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 block/blk-ioc.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 5dbcfa1b872e..57299f860d41 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -107,9 +107,23 @@ static void ioc_release_fn(struct work_struct *work)
 			ioc_destroy_icq(icq);
 			spin_unlock(&q->queue_lock);
 		} else {
-			spin_unlock_irq(&ioc->lock);
-			cpu_relax();
-			spin_lock_irq(&ioc->lock);
+			/* Make sure q and icq cannot be freed. */
+			rcu_read_lock();
+
+			/* Re-acquire the locks in the correct order. */
+			spin_unlock(&ioc->lock);
+			spin_lock(&q->queue_lock);
+			spin_lock(&ioc->lock);
+
+			/*
+			 * The icq may have been destroyed when the ioc lock
+			 * was released.
+			 */
+			if (!(icq->flags & ICQ_DESTROYED))
+				ioc_destroy_icq(icq);
+
+			spin_unlock(&q->queue_lock);
+			rcu_read_unlock();
 		}
 	}
 
-- 
2.20.1

Re: [PATCH 1/2] block: remove unnecessary ioc nested locking

[Daniel Wagner]

On Fri, Jun 19, 2020 at 05:23:17PM +0206, John Ogness wrote:
> The legacy CFQ IO scheduler could call put_io_context() in its exit_icq()
> elevator callback. This led to a lockdep warning, which was fixed in
> commit d8c66c5d5924 ("block: fix lockdep warning on io_context release
> put_io_context()") by using a nested subclass for the ioc spinlock.
> However, with commit f382fb0bcef4 ("block: remove legacy IO schedulers")
> the CFQ IO scheduler no longer exists.
> 
> The BFQ IO scheduler also implements the exit_icq() elevator callback but
> does not call put_io_context().
> 
> The nested subclass for the ioc spinlock is no longer needed. Since it
> existed as an exception and no longer applies, remove the nested subclass
> usage.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

As far I can tell, looks good.

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 2/2] block: remove retry loop in ioc_release_fn()

[Daniel Wagner]

On Fri, Jun 19, 2020 at 05:23:18PM +0206, John Ogness wrote:
> The reverse-order double lock dance in ioc_release_fn() is using a
> retry loop. This is a problem on PREEMPT_RT because it could preempt
> the task that would release q->queue_lock and thus live lock in the
> retry loop.
> 
> RCU is already managing the freeing of the request queue and icq. If
> the trylock fails, use RCU to guarantee that the request queue and
> icq are not freed and re-acquire the locks in the correct order,
> allowing forward progress.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Again, after starring on it for while and reading up and down,
I'd say, it looks good. Also a quick test run with blktests and
lockdep enabled didn't produce any warnings.

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 0/2] block: remove retry loop

[Jens Axboe]

On 6/19/20 9:17 AM, John Ogness wrote:
> Hello,
> 
> This series removes a retry loop in the ioc reverse-order
> double lock dance, replacing it with an implementation that
> uses guaranteed forward progress.
> 
> While at it, it also removes the nested spinlock usage,
> which no longer applies since CFQ is gone.

Applied for 5.9, thanks.

-- 
Jens Axboe