[block,regression] kernel oops triggered by removing scsi device dring IO

Commit Message

Bart Van Assche April 9, 2018, 4:47 a.m. UTC

On Sun, 2018-04-08 at 12:21 +0800, Ming Lei wrote:
> The following kernel oops is triggered by 'removing scsi device' during
> heavy IO.

Is the below patch sufficient to fix this?

Thanks,

Bart.


Subject: blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

Because blkcg_exit_queue() is now called from inside blk_cleanup_queue()
it is no longer safe to access cgroup information during or after the
blk_cleanup_queue() call. Hence check earlier in generic_make_request()
whether the queue has been marked as "dying".
---
 block/blk-core.c | 72 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

-- 
2.16.2

Comments

Joseph Qi April 9, 2018, 6:54 a.m. UTC | #1

Hi Bart,

On 18/4/9 12:47, Bart Van Assche wrote:
> On Sun, 2018-04-08 at 12:21 +0800, Ming Lei wrote:
>> The following kernel oops is triggered by 'removing scsi device' during
>> heavy IO.
> 
> Is the below patch sufficient to fix this?
> 
> Thanks,
> 
> Bart.
> 
> 
> Subject: blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash
> 
> Because blkcg_exit_queue() is now called from inside blk_cleanup_queue()
> it is no longer safe to access cgroup information during or after the
> blk_cleanup_queue() call. Hence check earlier in generic_make_request()
> whether the queue has been marked as "dying".

The oops happens during generic_make_request_checks(), in
blk_throtl_bio() exactly.
So if we want to bypass dying queue, we have to check this before
generic_make_request_checks(), I think.

Thanks,
Joseph

> ---
>  block/blk-core.c | 72 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index aa8c99fae527..3ac9dd25e04e 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2385,10 +2385,21 @@ blk_qc_t generic_make_request(struct bio *bio)
>  	 * yet.
>  	 */
>  	struct bio_list bio_list_on_stack[2];
> +	blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
> +		BLK_MQ_REQ_NOWAIT : 0;
> +	struct request_queue *q = bio->bi_disk->queue;
>  	blk_qc_t ret = BLK_QC_T_NONE;
>  
>  	if (!generic_make_request_checks(bio))
> -		goto out;
> +		return ret;
> +
> +	if (blk_queue_enter(q, flags) < 0) {
> +		if (unlikely(!blk_queue_dying(q) && (bio->bi_opf & REQ_NOWAIT)))
> +			bio_wouldblock_error(bio);
> +		else
> +			bio_io_error(bio);
> +		return ret;
> +	}
>  
>  	/*
>  	 * We only want one ->make_request_fn to be active at a time, else
> @@ -2423,46 +2434,37 @@ blk_qc_t generic_make_request(struct bio *bio)
>  	bio_list_init(&bio_list_on_stack[0]);
>  	current->bio_list = bio_list_on_stack;
>  	do {
> -		struct request_queue *q = bio->bi_disk->queue;
> -		blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
> -			BLK_MQ_REQ_NOWAIT : 0;
> -
> -		if (likely(blk_queue_enter(q, flags) == 0)) {
> -			struct bio_list lower, same;
> -
> -			/* Create a fresh bio_list for all subordinate requests */
> -			bio_list_on_stack[1] = bio_list_on_stack[0];
> -			bio_list_init(&bio_list_on_stack[0]);
> -			ret = q->make_request_fn(q, bio);
> -
> -			blk_queue_exit(q);
> -
> -			/* sort new bios into those for a lower level
> -			 * and those for the same level
> -			 */
> -			bio_list_init(&lower);
> -			bio_list_init(&same);
> -			while ((bio = bio_list_pop(&bio_list_on_stack[0])) != NULL)
> -				if (q == bio->bi_disk->queue)
> -					bio_list_add(&same, bio);
> -				else
> -					bio_list_add(&lower, bio);
> -			/* now assemble so we handle the lowest level first */
> -			bio_list_merge(&bio_list_on_stack[0], &lower);
> -			bio_list_merge(&bio_list_on_stack[0], &same);
> -			bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
> -		} else {
> -			if (unlikely(!blk_queue_dying(q) &&
> -					(bio->bi_opf & REQ_NOWAIT)))
> -				bio_wouldblock_error(bio);
> +		struct bio_list lower, same;
> +
> +		WARN_ON_ONCE(!(flags & BLK_MQ_REQ_NOWAIT) &&
> +			     (bio->bi_opf & REQ_NOWAIT));
> +		WARN_ON_ONCE(q != bio->bi_disk->queue);
> +		q = bio->bi_disk->queue;
> +		/* Create a fresh bio_list for all subordinate requests */
> +		bio_list_on_stack[1] = bio_list_on_stack[0];
> +		bio_list_init(&bio_list_on_stack[0]);
> +		ret = q->make_request_fn(q, bio);
> +
> +		/* sort new bios into those for a lower level
> +		 * and those for the same level
> +		 */
> +		bio_list_init(&lower);
> +		bio_list_init(&same);
> +		while ((bio = bio_list_pop(&bio_list_on_stack[0])) != NULL)
> +			if (q == bio->bi_disk->queue)
> +				bio_list_add(&same, bio);
>  			else
> -				bio_io_error(bio);
> -		}
> +				bio_list_add(&lower, bio);
> +		/* now assemble so we handle the lowest level first */
> +		bio_list_merge(&bio_list_on_stack[0], &lower);
> +		bio_list_merge(&bio_list_on_stack[0], &same);
> +		bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
>  		bio = bio_list_pop(&bio_list_on_stack[0]);
>  	} while (bio);
>  	current->bio_list = NULL; /* deactivate */
>  
>  out:
> +	blk_queue_exit(q);
>  	return ret;
>  }
>  EXPORT_SYMBOL(generic_make_request);
>

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index aa8c99fae527..3ac9dd25e04e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2385,10 +2385,21 @@  blk_qc_t generic_make_request(struct bio *bio)
 	 * yet.
 	 */
 	struct bio_list bio_list_on_stack[2];
+	blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
+		BLK_MQ_REQ_NOWAIT : 0;
+	struct request_queue *q = bio->bi_disk->queue;
 	blk_qc_t ret = BLK_QC_T_NONE;
 
 	if (!generic_make_request_checks(bio))
-		goto out;
+		return ret;
+
+	if (blk_queue_enter(q, flags) < 0) {
+		if (unlikely(!blk_queue_dying(q) && (bio->bi_opf & REQ_NOWAIT)))
+			bio_wouldblock_error(bio);
+		else
+			bio_io_error(bio);
+		return ret;
+	}
 
 	/*
 	 * We only want one ->make_request_fn to be active at a time, else
@@ -2423,46 +2434,37 @@  blk_qc_t generic_make_request(struct bio *bio)
 	bio_list_init(&bio_list_on_stack[0]);
 	current->bio_list = bio_list_on_stack;
 	do {
-		struct request_queue *q = bio->bi_disk->queue;
-		blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
-			BLK_MQ_REQ_NOWAIT : 0;
-
-		if (likely(blk_queue_enter(q, flags) == 0)) {
-			struct bio_list lower, same;
-
-			/* Create a fresh bio_list for all subordinate requests */
-			bio_list_on_stack[1] = bio_list_on_stack[0];
-			bio_list_init(&bio_list_on_stack[0]);
-			ret = q->make_request_fn(q, bio);
-
-			blk_queue_exit(q);
-
-			/* sort new bios into those for a lower level
-			 * and those for the same level
-			 */
-			bio_list_init(&lower);
-			bio_list_init(&same);
-			while ((bio = bio_list_pop(&bio_list_on_stack[0])) != NULL)
-				if (q == bio->bi_disk->queue)
-					bio_list_add(&same, bio);
-				else
-					bio_list_add(&lower, bio);
-			/* now assemble so we handle the lowest level first */
-			bio_list_merge(&bio_list_on_stack[0], &lower);
-			bio_list_merge(&bio_list_on_stack[0], &same);
-			bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
-		} else {
-			if (unlikely(!blk_queue_dying(q) &&
-					(bio->bi_opf & REQ_NOWAIT)))
-				bio_wouldblock_error(bio);
+		struct bio_list lower, same;
+
+		WARN_ON_ONCE(!(flags & BLK_MQ_REQ_NOWAIT) &&
+			     (bio->bi_opf & REQ_NOWAIT));
+		WARN_ON_ONCE(q != bio->bi_disk->queue);
+		q = bio->bi_disk->queue;
+		/* Create a fresh bio_list for all subordinate requests */
+		bio_list_on_stack[1] = bio_list_on_stack[0];
+		bio_list_init(&bio_list_on_stack[0]);
+		ret = q->make_request_fn(q, bio);
+
+		/* sort new bios into those for a lower level
+		 * and those for the same level
+		 */
+		bio_list_init(&lower);
+		bio_list_init(&same);
+		while ((bio = bio_list_pop(&bio_list_on_stack[0])) != NULL)
+			if (q == bio->bi_disk->queue)
+				bio_list_add(&same, bio);
 			else
-				bio_io_error(bio);
-		}
+				bio_list_add(&lower, bio);
+		/* now assemble so we handle the lowest level first */
+		bio_list_merge(&bio_list_on_stack[0], &lower);
+		bio_list_merge(&bio_list_on_stack[0], &same);
+		bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
 		bio = bio_list_pop(&bio_list_on_stack[0]);
 	} while (bio);
 	current->bio_list = NULL; /* deactivate */
 
 out:
+	blk_queue_exit(q);
 	return ret;
 }
 EXPORT_SYMBOL(generic_make_request);