linux-kernel - Re: [PATCH] blk-mq: release scheduler resource when request complete

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4fcddc26-4f51-4228-9a35-58dc1b5a16bc@kernel.dk>
Date:   Sun, 13 Aug 2023 07:54:23 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     chengming.zhou@...ux.dev, hch@....de, chuck.lever@...cle.com
Cc:     bvanassche@....org, cel@...nel.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, zhouchengming@...edance.com
Subject: Re: [PATCH] blk-mq: release scheduler resource when request complete

On 8/13/23 7:36 AM, chengming.zhou@...ux.dev wrote:
> From: Chengming Zhou <zhouchengming@...edance.com>
> 
> Chuck reported [1] a IO hang problem on NFS exports that reside on SATA
> devices and bisected to commit 615939a2ae73 ("blk-mq: defer to the normal
> submission path for post-flush requests").
> 
> We analysed the IO hang problem, found there are two postflush requests
> are waiting for each other.
> 
> The first postflush request completed the REQ_FSEQ_DATA sequence, so go to
> the REQ_FSEQ_POSTFLUSH sequence and added in the flush pending list, but
> failed to blk_kick_flush() because of the second postflush request which
> is inflight waiting in scheduler queue.
> 
> The second postflush waiting in scheduler queue can't be dispatched because
> the first postflush hasn't released scheduler resource even though it has
> completed by itself.
> 
> Fix it by releasing scheduler resource when the first postflush request
> completed, so the second postflush can be dispatched and completed, then
> make blk_kick_flush() succeed.

Change looks good to me. But since we're in here:

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index f14b8669ac69..5b14f18f9670 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -682,6 +682,15 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
>  }
>  EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);
>  
> +static void blk_mq_finish_request(struct request *rq)
> +{
> +	struct request_queue *q = rq->q;
> +
> +	if ((rq->rq_flags & RQF_USE_SCHED) &&
> +	    q->elevator->type->ops.finish_request)
> +		q->elevator->type->ops.finish_request(rq);
> +}

Any IO scheduler should set ->finish_request(), so this should just be:

static void blk_mq_finish_request(struct request *rq)
{
	struct request_queue *q = rq->q;

	if (rq->rq_flags & RQF_USE_SCHED)
		q->elevator->type->ops.finish_request(rq);
}

and would probably be a good idea to solidify that with a:

	if (WARN_ON_ONCE(!e->ops.finish_request))
		return -EINVAL;

at the top of elv_register() like we have for insert/dispatch as well.
All 3 IO schedulers do set ->finish_request().

-- 
Jens Axboe