Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231828 > unrolled thread

Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism Ming Lei <tom.leiming@gmail.com> - 2015-09-24 05:30 +0200
    Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism Tejun Heo <tj@kernel.org> - 2015-09-24 19:00 +0200
      Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism Tejun Heo <tj@kernel.org> - 2015-09-24 20:00 +0200
        Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism Tejun Heo <tj@kernel.org> - 2015-09-24 20:20 +0200
          Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism Tejun Heo <tj@kernel.org> - 2015-09-25 01:00 +0200

#1231828 — Re: [PATCH 3/3] blk-mq: Fix the queue freezing mechanism

On Wed, 23 Sep 2015 15:14:10 -0700
Bart Van Assche <bart.vanassche@sandisk.com> wrote:

> Ensure that blk_mq_queue_enter() waits if mq_freeze_depth is not
> zero. Ensure that the update of mq_freeze_depth by blk_mq_freeze_queue()
> is visible by all CPU cores before that function waits on
> mq_usage_counter.
> 
> It is unfortunate that this patch introduces an smp_mb() in the
> hot path (blk_mq_queue_enter()) but I have not yet found a way to
> avoid this.
> 
> I came across this code while analyzing a lockup triggered by
> deleting a SCSI host created by the SRP initiator immediately
> followed by a relogin.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: <stable@vger.kernel.org>
> ---
>  block/blk-mq.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 2077f0d..e3ad411 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -83,8 +83,13 @@ static int blk_mq_queue_enter(struct request_queue *q, gfp_t gfp)
>  	while (true) {
>  		int ret;
>  
> -		if (percpu_ref_tryget_live(&q->mq_usage_counter))
> -			return 0;
> +		if (percpu_ref_tryget_live(&q->mq_usage_counter)) {
> +			/* Order mq_use_counter and mq_freeze_depth accesses */
> +			smp_mb();
> +			if (!atomic_read(&q->mq_freeze_depth))
> +				return 0;
> +			percpu_ref_put(&q->mq_usage_counter);
> +		}

IMO, mq_freeze_depth should only be accessed in slow path, and looks
the race just happens during the small window between increasing
'mq_freeze_depth' and killing the percpu counter.

One solution I thought of is the following patch, which depends on
Akinobu's patch (blk-mq: fix freeze queue race
http://marc.info/?l=linux-kernel&m=143723697010781&w=2).

---
diff --git a/block/blk-mq.c b/block/blk-mq.c
index f774f67..1c71c04 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -77,6 +77,17 @@ static void blk_mq_hctx_clear_pending(struct blk_mq_hw_ctx *hctx,
 	clear_bit(CTX_TO_BIT(hctx, ctx), &bm->word);
 }
 
+static inline int blk_mq_read_freeze_depth(struct request_queue *q)
+{
+	int  depth;
+
+	mutex_lock(&q->mq_freeze_lock);
+	depth = q->mq_freeze_depth;
+	mutex_unlock(&q->mq_freeze_lock);
+
+	return depth;
+}
+
 static int blk_mq_queue_enter(struct request_queue *q, gfp_t gfp)
 {
 	while (true) {
@@ -89,7 +100,7 @@ static int blk_mq_queue_enter(struct request_queue *q, gfp_t gfp)
 			return -EBUSY;
 
 		ret = wait_event_interruptible(q->mq_freeze_wq,
-				!atomic_read(&q->mq_freeze_depth) ||
+				!blk_mq_read_freeze_depth(q) ||
 				blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
@@ -113,12 +124,9 @@ static void blk_mq_usage_counter_release(struct percpu_ref *ref)
 
 void blk_mq_freeze_queue_start(struct request_queue *q)
 {
-	int freeze_depth;
-
 	mutex_lock(&q->mq_freeze_lock);
 
-	freeze_depth = atomic_inc_return(&q->mq_freeze_depth);
-	if (freeze_depth == 1) {
+	if (!q->mq_freeze_depth++) {
 		percpu_ref_kill(&q->mq_usage_counter);
 		blk_mq_run_hw_queues(q, false);
 	}
@@ -149,7 +157,7 @@ void blk_mq_unfreeze_queue(struct request_queue *q)
 
 	mutex_lock(&q->mq_freeze_lock);
 
-	freeze_depth = atomic_dec_return(&q->mq_freeze_depth);
+	freeze_depth = --q->mq_freeze_depth;
 	WARN_ON_ONCE(freeze_depth < 0);
 	if (!freeze_depth) {
 		percpu_ref_reinit(&q->mq_usage_counter);
@@ -2084,7 +2092,7 @@ void blk_mq_free_queue(struct request_queue *q)
 /* Basically redo blk_mq_init_queue with queue frozen */
 static void blk_mq_queue_reinit(struct request_queue *q)
 {
-	WARN_ON_ONCE(!atomic_read(&q->mq_freeze_depth));
+	WARN_ON_ONCE(!ACCESS_ONCE(q->mq_freeze_depth));
 
 	blk_mq_sysfs_unregister(q);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6cdf2b7..86fedcc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -436,7 +436,7 @@ struct request_queue {
 	struct mutex		sysfs_lock;
 
 	int			bypass_depth;
-	atomic_t		mq_freeze_depth;
+	int			mq_freeze_depth;
 
 #if defined(CONFIG_BLK_DEV_BSG)
 	bsg_job_fn		*bsg_job_fn;



>  
>  		if (!(gfp & __GFP_WAIT))
>  			return -EBUSY;
> @@ -136,6 +141,11 @@ static void blk_mq_freeze_queue_wait(struct request_queue *q)
>  void blk_mq_freeze_queue(struct request_queue *q)
>  {
>  	blk_mq_freeze_queue_start(q);
> +	/*
> +	 * Ensure that the mq_freeze_depth update is visiable before
> +	 * mq_use_counter is read.
> +	 */
> +	smp_mb();
>  	blk_mq_freeze_queue_wait(q);
>  }
>  EXPORT_SYMBOL_GPL(blk_mq_freeze_queue);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1232254

On Thu, Sep 24, 2015 at 09:43:48AM -0700, Bart Van Assche wrote:
> On 09/23/2015 08:23 PM, Ming Lei wrote:
> >IMO, mq_freeze_depth should only be accessed in slow path, and looks
> >the race just happens during the small window between increasing
> >'mq_freeze_depth' and killing the percpu counter.
> 
> Hello Ming,
> 
> My concern is that *not* checking mq_freeze_depth in the hot path can cause
> a livelock. If there is a software layer, e.g. multipathd, that periodically
> submits new commands and if these commands take time to process e.g. because
> the transport layer is unavailable, how to guarantee that freezing ever
> succeeds without checking mq_freeze_depth in the hot path ?

I couldn't tell what the patch was trying to do from the patch
description, so including the above prolly is a good idea.  Isn't the
above guaranteed by percpu_ref_kill() preventing new tryget_live()'s?
Also, what does the barriers do in your patch?

The only race condition that I can see there is if unfreeze and freeze
race each other and freeze tries to kill the ref which hasn't finished
reinit yet.  We prolly want to put mutexes around freeze/unfreeze so
that they're serialized if something like that can happen (it isn't a
hot path to begin with).

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232313

Hello, Bart.

On Thu, Sep 24, 2015 at 10:35:41AM -0700, Bart Van Assche wrote:
> My interpretation of the percpu_ref_tryget_live() implementation in
> <linux/percpu-refcount.h> is that the tryget operation will only fail if the
> refcount is in atomic mode and additionally the __PERCPU_REF_DEAD flag has
> been set.

Yeah and percpu_ref_kill() does both.

> >Also, what does the barriers do in your patch?
> 
> My intention was to guarantee that on architectures that do not provide the
> same ordering guarantees as x86 (e.g. PPC or ARM) that the store and load
> operations on mq_freeze_depth and mq_usage_counter would not be reordered.
> However, it is probably safe to leave out the barrier I proposed to
> introduce in blk_mq_queue_enter() since it is acceptable that there is some
> delay in communicating mq_freeze_depth updates from the CPU that modified
> that counter to the CPU that reads that counter.

Hmmm... please don't use barriers this way.  Use it only when there's
a clear requirement for interlocking writer and reader pair.  There
isn't one here.  All it does is confusing people trying to read the
code.

> >The only race condition that I can see there is if unfreeze and freeze
> >race each other and freeze tries to kill the ref which hasn't finished
> >reinit yet.  We prolly want to put mutexes around freeze/unfreeze so
> >that they're serialized if something like that can happen (it isn't a
> >hot path to begin with).
> 
> My concern is that the following could happen if mq_freeze_depth is not
> checked in the hot path of blk_mq_queue_enter():
> * mq_usage_counter >= 1 before blk_mq_freeze_queue() is called.
> * blk_mq_freeze_queue() keeps waiting forever if new requests are queued
>   faster than that these requests complete.

Again, that doesn't happen.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232323

Hello,

On Thu, Sep 24, 2015 at 11:09:33AM -0700, Bart Van Assche wrote:
> On 09/24/2015 10:49 AM, Tejun Heo wrote:
> > Again, that doesn't happen.
> 
> Hello Tejun,
> 
> In case anyone would be interested, the backtraces for the lockup I had
> observed are as follows:

If this is happening and it's not caused by a hung in-flight request,
it's either percpu_ref being buggy or the forementioned kill/reinit
race screwing it up.  percpu_ref_kill() is expected to disable
tryget_live() in a finite amount of time regardless of concurrent
tryget tries.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232484

Hello, Bart.

On Thu, Sep 24, 2015 at 03:54:18PM -0700, Bart Van Assche wrote:
> Sorry that I had not yet made this clear but I agreed with the analysis in
> your two most recent e-mails. I think I have found the cause of the loop:
> for one or another reason the scsi_dh_alua driver was not loaded
> automatically. I think that caused the SCSI core to return a retryable error
> code for reads and writes sent over paths in the SCSI ALUA state "standby"
> instead of a non-retryable error code and that that caused the dm-mpath
> driver to enter an infinite loop. Loading the scsi_dh_alua driver resolved
> the infinite loop. Anyway, thank you for the feedback.

Great!  I think we still probably wanna fix the kill/reinit race tho.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web