Re: [PATCH RESEND v2 1/2] blk-mq: Fix failed allocation path when mapping queues

[Douglas Miller <dougmill@linux.vnet.ibm.com>]

On 12/06/2016 09:31 AM, Gabriel Krisman Bertazi wrote:
> In blk_mq_map_swqueue, there is a memory optimization that frees the
> tags of a queue that has gone unmapped.  Later, if that hctx is remapped
> after another topology change, the tags need to be reallocated.
>
> If this allocation fails, a simple WARN_ON triggers, but the block layer
> ends up with an active hctx without any corresponding set of tags.
> Then, any income IO to that hctx can trigger an Oops.
>
> I can reproduce it consistently by running IO, flipping CPUs on and off
> and eventually injecting a memory allocation failure in that path.
>
> In the fix below, if the system experiences a failed allocation of any
> hctx's tags, we remap all the ctxs of that queue to the hctx_0, which
> should always keep it's tags.  There is a minor performance hit, since
> our mapping just got worse after the error path, but this is
> the simplest solution to handle this error path.  The performance hit
> will disappear after another successful remap.
>
> I considered dropping the memory optimization all together, but it
> seemed a bad trade-off to handle this very specific error case.
>
> This should apply cleanly on top of Jen's for-next branch.
>
> The Oops is the one below:
>
> SP (3fff935ce4d0) is in userspace
> 1:mon> e
> cpu 0x1: Vector: 300 (Data Access) at [c000000fe99eb110]
>      pc: c0000000005e868c: __sbitmap_queue_get+0x2c/0x180
>      lr: c000000000575328: __bt_get+0x48/0xd0
>      sp: c000000fe99eb390
>     msr: 900000010280b033
>     dar: 28
>   dsisr: 40000000
>    current = 0xc000000fe9966800
>    paca    = 0xc000000007e80300   softe: 0        irq_happened: 0x01
>      pid   = 11035, comm = aio-stress
> Linux version 4.8.0-rc6+ (root@bean) (gcc version 5.4.0 20160609
> (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.2) ) #3 SMP Mon Oct 10 20:16:53 CDT 2016
> 1:mon> s
> [c000000fe99eb3d0] c000000000575328 __bt_get+0x48/0xd0
> [c000000fe99eb400] c000000000575838 bt_get.isra.1+0x78/0x2d0
> [c000000fe99eb480] c000000000575cb4 blk_mq_get_tag+0x44/0x100
> [c000000fe99eb4b0] c00000000056f6f4 __blk_mq_alloc_request+0x44/0x220
> [c000000fe99eb500] c000000000570050 blk_mq_map_request+0x100/0x1f0
> [c000000fe99eb580] c000000000574650 blk_mq_make_request+0xf0/0x540
> [c000000fe99eb640] c000000000561c44 generic_make_request+0x144/0x230
> [c000000fe99eb690] c000000000561e00 submit_bio+0xd0/0x200
> [c000000fe99eb740] c0000000003ef740 ext4_io_submit+0x90/0xb0
> [c000000fe99eb770] c0000000003e95d8 ext4_writepages+0x588/0xdd0
> [c000000fe99eb910] c00000000025a9f0 do_writepages+0x60/0xc0
> [c000000fe99eb940] c000000000246c88 __filemap_fdatawrite_range+0xf8/0x180
> [c000000fe99eb9e0] c000000000246f90 filemap_write_and_wait_range+0x70/0xf0
> [c000000fe99eba20] c0000000003dd844 ext4_sync_file+0x214/0x540
> [c000000fe99eba80] c000000000364718 vfs_fsync_range+0x78/0x130
> [c000000fe99ebad0] c0000000003dd46c ext4_file_write_iter+0x35c/0x430
> [c000000fe99ebb90] c00000000038c280 aio_run_iocb+0x3b0/0x450
> [c000000fe99ebce0] c00000000038dc28 do_io_submit+0x368/0x730
> [c000000fe99ebe30] c000000000009404 system_call+0x38/0xec
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
> Cc: Brian King <brking@linux.vnet.ibm.com>
> Cc: Douglas Miller <dougmill@linux.vnet.ibm.com>
> Cc: linux-block@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> ---
>   block/blk-mq.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 6fb94bd69375..6718f894fbe1 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1870,7 +1870,7 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
>   static void blk_mq_map_swqueue(struct request_queue *q,
>   			       const struct cpumask *online_mask)
>   {
> -	unsigned int i;
> +	unsigned int i, hctx_idx;
>   	struct blk_mq_hw_ctx *hctx;
>   	struct blk_mq_ctx *ctx;
>   	struct blk_mq_tag_set *set = q->tag_set;
> @@ -1893,6 +1893,15 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>   		if (!cpumask_test_cpu(i, online_mask))
>   			continue;
>
> +		hctx_idx = q->mq_map[i];
> +		/* unmapped hw queue can be remapped after CPU topo changed */
> +		if (!set->tags[hctx_idx]) {
> +			set->tags[hctx_idx] = blk_mq_init_rq_map(set, hctx_idx);
> +
> +			if (!set->tags[hctx_idx])
> +				q->mq_map[i] = 0;
> +		}
> +
>   		ctx = per_cpu_ptr(q->queue_ctx, i);
>   		hctx = blk_mq_map_queue(q, i);
>
> @@ -1909,7 +1918,10 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>   		 * disable it and free the request entries.
>   		 */
>   		if (!hctx->nr_ctx) {
> -			if (set->tags[i]) {
> +			/* Never unmap queue 0.  We need it as a
> +			 * fallback in case of a new remap fails
> +			 * allocation. */
> +			if (i && set->tags[i]) {
>   				blk_mq_free_rq_map(set, set->tags[i], i);
>   				set->tags[i] = NULL;
>   			}
> @@ -1917,11 +1929,8 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>   			continue;
>   		}
>
> -		/* unmapped hw queue can be remapped after CPU topo changed */
> -		if (!set->tags[i])
> -			set->tags[i] = blk_mq_init_rq_map(set, i);
>   		hctx->tags = set->tags[i];
> -		WARN_ON(!hctx->tags);
> +		BUG_ON(!hctx->tags);
>
>   		/*
>   		 * Set the map size to the number of mapped software queues.
Signed-off-by: Douglas Miller <dougmill@linux.vnet.ibm.com>